Skip to content

FIx coverage for generate scopes #1184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 20, 2025
Merged

FIx coverage for generate scopes #1184

merged 3 commits into from
May 20, 2025

Conversation

Blebowski
Copy link
Contributor

Hi,

this is a proposal for fixing the generate scopes not being emitted as coverage "instance" scopes.
It works well for for generate because T_BLOCK name for the generate has iterator variable suffix.

In case of if generate or case generate, only a single block is elaborated at single simulation.
A branch / choice inherits name of the parent generate statement, unless overridden by a branch / choice
specific alternative_label. If user omits alternative_label, than different HDL items may end up with
the same code coverage item name (in a different elaborations / simulations). When merging the code
coverage data from such simulations, such data aliases.

Consider e.g. following example:

G_CASE_GEN : case (G_PAR) generate
    when 0 => c <= '1';
    when 1 => d <= '0';
    when 2 => e <= '1';
    when 3 => f <= '0';
    end generate;

Where G_PAR is a top level generic of the entity instantiating this case generate statement.
Upon elaborations with G_PAR ranging from 0 to 3, each of the signal assign statements from
c up to f will have following coverage item name:

<scope name="G_CASE_GEN" block_name="G_CASE_GEN" line="31">
      <scope name="_P0">
        <scope name="_S0">
          <statement hier="WORK.COVER27.G_CASE_GEN._P0._S0" data="1"/>
        </scope>
      </scope>
    </scope>

since the implicit statement / process / branch / expression labelling always counts the equally
within distinct coverage scopes. Merging of code coverage from such simulations gives confusing
results and false positives.

An obvious approach how to fix this, would be appending the branch / choice index to a name
of implicit block that is created for such choice / branch. Such "conflict avoidance" would work
very similarly to implicit labelling of statements / processes / expressions for code coverage.
Do you think it is better to append such "implicit counter" to the name of the created code coverage
scope only, or to the name of the implicit block ? Or would you choose another approach ?

@Blebowski Blebowski marked this pull request as draft April 7, 2025 21:14
@Blebowski Blebowski force-pushed the cover-fix-generate-scope branch 4 times, most recently from 1c73d35 to 760d0f6 Compare April 11, 2025 06:58
@Blebowski Blebowski marked this pull request as ready for review April 11, 2025 09:11
@Blebowski
Copy link
Contributor Author

Hi,
I decided not to deal with it. After thinking about solution, this would lead to tracking index of the
if/else generate branch or case generate alternative from the parser through the whole front-end
up to code emit. Querying the index would not work in the "elab/lower" phase, since folded false
branches / alternatives can "drop" during simplification.

Instead, I added loc compare when determining if two items to be merged are equal. This will
solve the most obvious problems. I extended the manual with some limitations so that users can
avoid aliasing of coverage items.

@nickg do you have any way on how to debug the failing test on MAC OS ? I don't have MACOS
machine.

@Blebowski Blebowski force-pushed the cover-fix-generate-scope branch 9 times, most recently from 67ad74b to 5de1e23 Compare April 17, 2025 08:20
@Blebowski
Copy link
Contributor Author

Hi,
this is now ready for review. The issue was in SHA Library. Despite SHA1_Update buffer being defined as const,
the f-tion actually changes the string content. I think it happens in the default non-accelerated implementation
(MACOS x86) where blk(i) macro changes the underlying block which is a pointer to the string.

@Blebowski Blebowski force-pushed the cover-fix-generate-scope branch 2 times, most recently from 0fcba62 to f018509 Compare April 27, 2025 17:41
@Blebowski
Copy link
Contributor Author

Hi @nickg ,
do you have any opinion on this MR please ?
Are there some changes (e.g. in the approach to the test) you want me to apply ?

nickg added a commit that referenced this pull request May 18, 2025
@nickg
Fix non-accelerated SHA1 writing to input buffer
5f47fdb 
Spotted by @Blebowski in #1184
@nickg
Copy link
Owner

nickg commented May 18, 2025

I don't think this is really a problem: if the user wants the path names to be distinct he can use the alternative labels which are provided by the language for exactly this purpose. I don't like using the locations for anything that's not just information displayed to the user.

I think it happens in the default non-accelerated implementation (MACOS x86) where blk(i) macro changes the underlying block which is a pointer to the string.

Yeah I guess it's a bug introduced when I added the accelerated versions. Fixed it in 5f47fdb.

@Blebowski
Copy link
Contributor Author

OK. I will remove the loc compare then and update the manual entry.
I think it may be good to emit a warning if coverage is enabled and
there is case generate choice or if-generate-else branch without alternative label.

@Blebowski Blebowski force-pushed the cover-fix-generate-scope branch from f018509 to d61cd37 Compare May 18, 2025 15:44
@nickg nickg force-pushed the cover-fix-generate-scope branch from d61cd37 to c336ac9 Compare May 20, 2025 08:03
@nickg nickg merged commit 7f2fedd into nickg:master May 20, 2025
13 checks passed
nickg added a commit that referenced this pull request Jun 4, 2025
@nickg
Fix non-accelerated SHA1 writing to input buffer
85152db 
Spotted by @Blebowski in #1184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickg nickg nickg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Blebowski @nickg